Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bug] Fix name collision in ti.dataclass #6737

Merged
merged 3 commits into from
Dec 5, 2022

Conversation

strongoier
Copy link
Contributor

@strongoier strongoier commented Nov 25, 2022

Issue: fix #6652

Brief Summary

Properties should be added to a certain Struct instance instead of the Struct class itself.

@netlify
Copy link

netlify bot commented Nov 25, 2022

Deploy Preview for docsite-preview ready!

Name Link
🔨 Latest commit 7c1054e
🔍 Latest deploy log https://app.netlify.com/sites/docsite-preview/deploys/6389c88e5890a8000897eb59
😎 Deploy Preview https://deploy-preview-6737--docsite-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Contributor

@ailzhang ailzhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@YouJiacheng
Copy link

YouJiacheng commented Dec 14, 2022

Use __getattr__ and __setattr__ might be a better choice than create a new class for each instance.
And that will break user's code like (fortunately, taichi use isinstance internally)

foo = ti.Struct(...)
if type(foo) is ti.Struct:
    # do something
    pass

Use __getattr__ and __setattr__ can remove boilerplates _make_getter and _make_setter as well. (BTW, why setter requires python scope while __setitem__ doesn't?)

If setter has the same requirements as __setitem__:

class Struct(TaichiOperations):
    # remove _register_members, _make_getter and _make_setter
    def __getattr__(self, name):
        try:
            return self[name]
        except KeyError:
            raise AttributeError(f"'{self.__class__.__name__}' object has no attribute '{name}'") from None
     def __setattr__(self, name, value):
         if name not in self.entries:
             return super().__setattr__(name, value)
         self[name] = value

This implementation has an additional benefit: raise correct Error(AttributeError instead of KeyError) after entries modified by users.(entries is a public field thus subject to change)

@strongoier
Copy link
Contributor Author

Sounds good! @YouJiacheng Are you interested in sending a PR for this?

(BTW, why setter requires python scope while setitem doesn't?)

Because in taichi scope __setitem__ will never be called. The assignment will be handled by the AST transformer.

@YouJiacheng
Copy link

Hmmm, IIUC attribute assignment will be handled by the AST transformer as well. I will send a PR for this after my final exam(2023.1.4).

quadpixels pushed a commit to quadpixels/taichi that referenced this pull request May 13, 2023
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Twoti.dataclass member/method name collision
3 participants